Fixing the regression in inner hits aggregation #13488
Conversation
Signed-off-by: Ankit Jain <[email protected]>
Signed-off-by: Ankit Jain <[email protected]>
jainankitk
requested review from
anasalkouz,
andrross,
Bukhtawar,
CEHENKLE,
dblock,
dbwiddis,
dreamer-89,
gbbafna,
kotwanikunal,
mch2,
msfroh,
nknize,
owaiskazi19,
reta,
Rishikesh1159,
sachinpkale,
saratvemulapalli,
shwetathareja,
sohami,
tlfeng and
VachaShah
as code owners
github-actions
bot
added
bug
|
Related to #13486 |
I am not comfortable with this change as is - we need to add more tests, not just one, to cover all uncovered flows and catch the regressions. |
Signed-off-by: Ankit Jain <[email protected]>
Signed-off-by: Ankit Jain <[email protected]>
@dblock @reta - Ideally the original commit should have been 2 separate PRs one for ScriptFields and InnerHits. My suggestion is to not press panic button and revert everything. Instead make more cognitive choice.
|
hence we have such regressions, I prefer taking more time to understand were we have misses instead of patching in a rush |
|
❌ Gradle check result for bbdbc08: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
|
❌ Gradle check result for 45ee240: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
|
❌ Gradle check result for 23c45d5: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
As previously mentioned, those 2 changes should have gone as part of separate commits, and makes sense to revert only one of those. But, I am okay if you really prefer to revert both the changes. |
Yeah... I think that makes sense. Let's get back to the known-okay behavior -- as far as I know, we don't have any evidence that initializing the (*) Taking a look at the |
|
@msfroh @reta decide in #13486 (comment)? |
|
Closing this PR in favor of #13486 |
This commit fixing the regression introduced by da5b205.
Description
Reverts #12503 that introduced regression in 2.13.0. Thanks a lot @martijnbolhuis for spotting it and supplying the tests, @dblock for working with the contributor.
Related Issues
Closes #13467
Check List
- [ ] Public documentation issue/PR createdBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.